Repository navigation
Fix strict mode for nested calls and align the parser with R operators - #7
Merged
Merged
Conversation
Minus is no longer part of names and signs are no longer part of numbers, so a-f(1), 1-1 and x -1 parse as in R. Add ^, %*%, %% and %/%, unary plus, and follow R precedence. Formulas use the expression grammar, so the response can be an expression, and still reject strings and function calls.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follows up on obiba/datashield4j#8, which fixes operator handling in the Java parsers. DSLite has the same grammar bugs, but with different effects: it writes the AST back as infix text and R re-parses it, so wrong grouping never changed a result. The bugs only caused valid R to be rejected. Checking this also turned up a separate strict mode bypass.
Changes
Strict mode (
R/DSLiteServer.R):replaceFunctionNoderecursed throughself$children, whereselfis the server rather than the node, so only the outermost call was checked and mapped. Withfallowed,f(system('id')),1 + system('id')andx = system('id')all passed. Nested calls are now checked and mapped.Lexer and grammar (
R/lex-yacc.R):-is no longer part of names, and signs are no longer part of numbers.a-f(1),a-(b),1-1,x -1and-1Lnow parse.^,%*%,%%,%/%and unary+are supported in expressions.?Syntax.formula_termgrammar is removed and~is an ordinary low-precedence operator.y-1 ~ xdepended on-being part of names, so it would otherwise stop parsing.y ~ -xandy ~ x -1now parse. Strings and function calls are still rejected in formulas.Behaviour changes
-1is now aUnaryOpNoderather than aNumericNode, andabc-DEFis a subtraction rather than a symbol. The evaluated result is unchanged, but code inspecting the AST would see the difference.a-bis emitted asa - b.Testing
%operators, formulas and the round trip through R's parser.parse()after the round trip. Before, 1,923 matched and 1,845 were rejected. None differed silently, before or after.parse(), run after the change only. These comparisons were one-off scripts and are not part of this PR.R CMD checkand the dsBaseClient test suite.Not addressed
These are valid R and are still rejected: comparison and logical operators, subsetting (
a[1]), one-sided formulas (~ x) and.5.f(D$x = 1)still gets past the DSLite parser and is rejected later byparse().